Skip to content

treewide: skip generating shell completions using $out/bin/… when cross compiling#326243

Merged
pbsds merged 6 commits intoNixOS:masterfrom
jcaesar:pr-13
Aug 6, 2024
Merged

treewide: skip generating shell completions using $out/bin/… when cross compiling#326243
pbsds merged 6 commits intoNixOS:masterfrom
jcaesar:pr-13

Conversation

@jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Jul 11, 2024

Description of changes

Fixes regressions for cross compiling introduced in #289517 on 61 packages. See #308283 for more discussion.

There are a lot of packages that use the following pattern to generate shell completions:

  postInstall = ''
     installShellCompletion --cmd foo \
       --bash <($out/bin/foo generate-completions bash) \

  '';

The crucial part is that it executes $out/bin/foo, which failed silently on cross builds so far (ignoring binfmt_misc here), but is now raised as a build error from installShellCompletion.

Most of what this PR does is prefix (that part of) those postInstall scripts with:

lib.optionalString (stdenv.buildPlatform.canExecute stdenv.hostPlatform)

Some additional notes:

  • This is limited to 61 packages, there are potentially 327 instances of this.
    • I'm not sure this will get merged and fixing ~300 packages manually is too much considering that.
    • They're all buildRustPackage, because I'm familiar with that ecosystem, and the completion generation logic is nearly all the same, so it was easy to investigate problems for me.
  • comodoro, attic-client, neverest, himalaya, colmena: Since I already stumbled upon them, it also changes some instances of buildPlatform == hostPlatform to buildPlatform.canExecute hostPlatform.
  • trashy: Move from preFixup to postInstall for consistency
  • charasay: Fix generation command
  • inlyne: Use checked-in completion scripts

Checking

nixpkgs-review isn't super insightful for this, since I took extra care to not introduce any changes, not even whitespace, where unnecessary:

Result of nixpkgs-review run on x86_64-linux 1

3 packages built:
  • charasay
  • inlyne
  • trashy

So, I also obtained a list of changed packages

 git diff-tree --no-commit-id --name-only -r HEAD~2 | each { path dirname | path basename } | str join " "

and tested that all build, with:

env NIXPKGS_ALLOW_UNFREE=1 nix shell --keep-going --impure -vL --expr 'let nixpkgs = builtins.getFlake "git+file:."; in with nixpkgs.legacyPackages.x86_64-linux.pkgsCross.aarch64-multiplatform; mkShell { packages = [comodoro flavours genact inlyne leetcode-cli git-absorb jujutsu youki ab-av1 ast-grep attic-client clipcat codeberg-cli engage himalaya joshuto neverest pace pixi qrtool release-plz sn0int steamguard-cli zola jrsonnet cocogitto diesel-cli espup fnm cargo-show-asm cauwugo typeshare sentry-cli snazy tokio-console volta  dufs colmena charasay dotter fd intermodal miniserve onefetch sheldon starship topgrade trashy twm zellij magic-wormhole-rs fh nix-template bws jwt-cli kbs2 prs rbw  shisho languagetool-rust termbook]; }'

(skipping maa-cli sheesy-cli deno tremor solana-cli elf2nucleus because they fail to compile or link.)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jul 11, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 11, 2024
@jcaesar jcaesar force-pushed the pr-13 branch 4 times, most recently from 5e9658d to 3bb551e Compare July 25, 2024 00:34
@jcaesar jcaesar marked this pull request as ready for review July 25, 2024 12:16
Copy link
Member

@pbsds pbsds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff LGTM sans one error, i also support this approach until someone rather decides to go the QEMU route and puts the work into actually implementing it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the presets below are not conditioned on canExecute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed.

Also rebased on master, and additionally had to fix the formatting of maa-cli now to get a green CI…

@pbsbot
Copy link

pbsbot commented Aug 3, 2024

Result of nixpkgs-review pr 326243 run on x86_64-linux 1

3 packages built:
  • charasay
  • inlyne
  • trashy

jcaesar added 2 commits August 4, 2024 10:42
upstream recommends using cargo xtask gen, but checks in the results
jcaesar added 3 commits August 4, 2024 10:50
…ss compiling

This focuses on Rust packages, since the most commonly used argument
parser library (clap/structopt) makes the following pattern natural and
thus common:

  postInstall = ''
    installShellCompletion --cmd foo \
      --bash <($out/bin/foo completion bash) \
      …

This commit just guards those with

lib.optionalString (stdenv.buildPlatform.canExecute stdenv.hostPlatform)

splitting the string where unrelated actions are performed.
…ecute

This rewrites uses of

stdenv.hostPlatform == stdenv.buildPlatform

to

stdenv.buildPlatform.canExecute stdenv.hostPlatform

when guarding postInstall scripts that use $out/bin/… to generate shell completions
@pbsbot
Copy link

pbsbot commented Aug 6, 2024

Result of nixpkgs-review pr 326243 run on x86_64-linux 1

4 packages built:
  • charasay
  • inlyne
  • starship
  • trashy

@pbsds pbsds merged commit 462b96d into NixOS:master Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants